fix(core): add explicit | undefined to Transport interface optional properties#1766
fix(core): add explicit | undefined to Transport interface optional properties#1766felixweinberger wants to merge 1 commit intomainfrom
Conversation
…roperties Adds | undefined to optional properties on the Transport interface and TransportSendOptions so the interface matches what tsc emits in .d.ts for implementing classes. This fixes TS2420 errors for consumers using exactOptionalPropertyTypes: true without skipLibCheck. Fixes #1314
🦋 Changeset detectedLatest commit: 1497412 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM -- straightforward type-level fix for exactOptionalPropertyTypes compatibility.
Extended reasoning...
Overview
This PR modifies a single source file (packages/core/src/shared/transport.ts) and adds a changeset. The change is purely at the type level: adding | undefined to optional properties on the Transport interface and one property on TransportSendOptions. This fixes TS2420 errors for consumers using exactOptionalPropertyTypes: true without skipLibCheck.
Security risks
None. This is a type-only change with zero runtime impact. No logic, control flow, or data handling is modified.
Level of scrutiny
Low scrutiny is appropriate. The change is mechanical and well-understood: under exactOptionalPropertyTypes, prop?: T and prop?: T | undefined have different semantics, and TypeScript's emitted .d.ts files for class implementations use the latter form. The PR aligns the interface declarations with what tsc emits, which is the correct fix.
Other factors
The bug report identifies two missed properties (relatedRequestId and resumptionToken in TransportSendOptions) that could also receive the | undefined treatment. This is a valid nit for consistency, but the practical impact is very low since TransportSendOptions is a type alias (not an interface implemented by classes), so it doesn't trigger the TS2420 error that motivated this PR. The core fix for the Transport interface is complete and correct. The PR also consolidates work from three prior PRs (#1507, #1661, #1736), includes a proper changeset, and the author confirms pnpm check:all passes.
| * | ||
| * This allows clients to persist the latest token for potential reconnection. | ||
| */ | ||
| onresumptiontoken?: (token: string) => void; | ||
| onresumptiontoken?: ((token: string) => void) | undefined; | ||
| }; |
There was a problem hiding this comment.
🟡 The | undefined fix for exactOptionalPropertyTypes was applied to onresumptiontoken in TransportSendOptions but not to the other two optional properties (relatedRequestId on line 55 and resumptionToken on line 62). For consistency, these should also get | undefined — a consumer passing { resumptionToken: undefined } would still get a type error under exactOptionalPropertyTypes: true.
Extended reasoning...
What the bug is
The PR adds | undefined to all optional properties on the Transport interface (onclose, onerror, onmessage, sessionId, setProtocolVersion, setSupportedProtocolVersions) and to onresumptiontoken in TransportSendOptions. However, it misses the other two optional properties in TransportSendOptions: relatedRequestId?: RequestId (line 55) and resumptionToken?: string (line 62). The changeset description even explicitly mentions TransportSendOptions as being fixed.
Concrete example
Under exactOptionalPropertyTypes: true, prop?: T means the property can be omitted but cannot be explicitly set to undefined. To allow explicit undefined, you need prop?: T | undefined. After this PR, a consumer writing:
const opts: TransportSendOptions = {
resumptionToken: undefined,
onresumptiontoken: myCallback
};would get a type error on resumptionToken: undefined because it is still typed as resumptionToken?: string (without | undefined), even though onresumptiontoken in the same object is now correctly typed. The test file at packages/client/test/client/streamableHttp.test.ts:1099 actually does pass resumptionToken: undefined in exactly this way.
Impact
The practical impact is low. TransportSendOptions is a type alias, not a class-implemented interface, so the primary TS2420 error that motivated this PR does not apply here. The issue only manifests if a consumer explicitly passes undefined for these properties (rather than simply omitting them) while using exactOptionalPropertyTypes: true. This is an uncommon pattern, but it is inconsistent to fix one property and not the others in the same type.
Suggested fix
Add | undefined to the remaining two optional properties in TransportSendOptions:
relatedRequestId?: RequestId | undefined;
resumptionToken?: string | undefined;This is a one-line-each change that makes the fix complete and consistent with the stated goal of the PR.
|
I pulled this branch locally and prepared a minimal follow-up for the missing enforcement piece: #1767 Why the follow-up:
The follow-up PR adds:
Verified locally with:
|
2 participants
Adds
| undefinedto optional properties on theTransportinterface andTransportSendOptionsso the interface matches what tsc emits in.d.tsfor implementing classes.Motivation and Context
Consumers using
exactOptionalPropertyTypes: truewithoutskipLibCheckhit TS2420 errors because the SDK's emitted.d.tsfor classes likeStreamableHTTPServerTransportincludes| undefinedon optional callbacks, but theTransportinterface does not.Fixes #1314
How Has This Been Tested?
pnpm check:allpasses. Verified the interface change resolves the TS2420 error by enablingexactOptionalPropertyTypeslocally against the test file.Breaking Changes
None — adding
| undefinedto?: Tis a widening under default TS settings (no-op) and a fix under strict settings.Types of changes
Checklist
Additional context
Thanks to @rechedev9 and @naman10parikh for the earlier PRs (#1661, #1736) exploring this fix. This PR consolidates the interface changes and adds
sessionIdandonresumptiontokenwhich were missed in the originals.Supersedes #1507, #1661, #1736.
Workaround for older SDK versions: enable
skipLibCheck: truein your tsconfig (recommended by TypeScript team and part of@tsconfig/strictest).